Added spinner on retrieving bind key in the client#388
Conversation
tuj
added
bug
turegjorup
left a comment
turegjorup
left a comment
There was a problem hiding this comment.
Refactor and the UNKNOWN/default fix look good — that case previously froze the polling loop, which is a real fix worth highlighting.
Bug
- Spinner shows on top of admin previews.
retrievingBindKeyis initialized totrueand only cleared instartContentand theREADY/AWAITING_BIND_KEYbranches. For non-screenpreview modes (slide,playlist,theme, …),useEffectskipsstartContentand never touches the flag, soretrievingBindKey && !bindKeystays true and the spinner sits in the bottom-right corner of every admin preview. Suggest gating on!previewor initializing frompreview === null. (app.jsx:299-303)
Nits
.catchincheckLogindoesn't clearretrievingBindKey, so a degraded API leaves the spinner running forever with no error surfaced. Probably intentional, but worth a comment. (app.jsx:148-150)- Test file
app-reauth.test.jsxcontains twodescribes, only one of which is about reauth — the other is aboutUNKNOWN-status polling. Rename or split. - Neither test actually asserts on the spinner element (the headline feature). One
expect(document.querySelector(".retrieving-bind-key-spinner")).toBeInTheDocument()before/after would close that gap. @keyframes spinis declared globally inapp.scss. No collision today, but a more specific name (e.g.bind-key-spin) avoids future ones.
turegjorup
left a comment
turegjorup
left a comment
There was a problem hiding this comment.
Posting the earlier findings as inline suggestions for easier action. See top-level review for rationale; comments below carry the concrete diffs.
| break; | ||
| } | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
Nit: the .catch handler below doesn't clear retrievingBindKey, so a degraded API leaves the spinner running indefinitely with no error surfaced. Probably intentional (we're still trying), but worth a one-line comment explaining the choice — or surfacing statusService.setError(...) on repeated failures.
| await act(async () => {}); | ||
| } | ||
|
|
||
| describe("checkLogin retries on unknown status", () => { |
There was a problem hiding this comment.
Nit: this describe is about polling on LOGIN_STATUS_UNKNOWN, not reauth — the file name app-reauth.test.jsx is misleading. Either rename the file (e.g. app-bind-key.test.jsx) or split the unknown-status case into its own file.
| await tick(0); | ||
|
|
||
| // Bind key should NOT be visible yet (unknown status, no bind key set). | ||
| expect(screen.queryByText("BIND42")).not.toBeInTheDocument(); |
There was a problem hiding this comment.
Nit: the headline feature of this PR is the spinner, but neither test asserts on the spinner element. Easy gap to close:
| expect(screen.queryByText("BIND42")).not.toBeInTheDocument(); | |
| expect(screen.queryByText("BIND42")).not.toBeInTheDocument(); | |
| expect(document.querySelector(".retrieving-bind-key-spinner")).toBeInTheDocument(); |
And a complementary not.toBeInTheDocument() after the bind key arrives at line 140.
Co-authored-by: Ture Gjørup <ture@ignatz.dk>
Co-authored-by: Ture Gjørup <ture@ignatz.dk>
2 participants
Link to issue
#380
Link to ticket
https://leantime.itkdev.dk/#/tickets/showTicket/7203
Description
Builds on #387
Added spinner on retrieving bind key in the client.
Checklist